Create HITL specific permission for core-API#54043
Conversation
sjyangkevin
left a comment
There was a problem hiding this comment.
Hi @Lee-W , I've created an initial draft, and want to see if I am on the right direction. Would you mind have a look on it when you have time and let me know if any of the comments here are not clear. I will try my best to explain my understanding and approach. Thanks!
airflow-core/src/airflow/api_fastapi/auth/managers/base_auth_manager.py
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/api_fastapi/auth/managers/models/resource_details.py
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/api_fastapi/auth/managers/models/resource_details.py
Show resolved
Hide resolved
airflow-core/src/airflow/api_fastapi/core_api/routes/public/hitl.py
Outdated
Show resolved
Hide resolved
providers/fab/src/airflow/providers/fab/auth_manager/fab_auth_manager.py
Outdated
Show resolved
Hide resolved
providers/fab/src/airflow/providers/fab/auth_manager/fab_auth_manager.py
Outdated
Show resolved
Hide resolved
d0568fb to
93e86f1
Compare
providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py
Outdated
Show resolved
Hide resolved
|
In the provider check, I keep seeing the following error. It looks like there is a compatibility issue with 3.0.3 after adding the I would like to provide a summary regarding my approach of adding a new permission, and I would appreciate if I could get some guidance on how to properly make it available for only after 3.1.0.
I think the proper approach is to have a check for Airflow version and then define/register this new permission only the version is 3.1.0. However, it looks like we need to introduce a lot of version check across core, providers, and tests.. Not sure if there is a better way to implement it. Thanks! |
93e86f1 to
e88fa4e
Compare
|
This is how I did the 3.1 check in another PR. and yes, we'll need to add this check in multiple places but I think we've already have most of them 🤔 is there anything missed? |
|
The main branch fixes a CI issue. Thus I'll rebase the branch from main and start my first round review to provide early feedback 🙂 |
e88fa4e to
036e99c
Compare
providers/fab/src/airflow/providers/fab/auth_manager/fab_auth_manager.py
Outdated
Show resolved
Hide resolved
Thanks, I just have a look into the #53035, and I think most of the changes related to permission are consistent with this PR. I think we also need to register that resource for dag access entity. Will try to add one and see if I can resolve the CI failure |
Thanks! I will make those adjustments, and attach more test evidence, as well as checking if the test cases needed to be adjusted. |
There was a problem hiding this comment.
Nice looking good beside compat check mentionned by Lee.
cc: @vincbeck
jason810496
left a comment
There was a problem hiding this comment.
Nice! Thanks for the PR and LGTM as well.
providers/fab/src/airflow/providers/fab/auth_manager/fab_auth_manager.py
Outdated
Show resolved
Hide resolved
036e99c to
75d7756
Compare
… redundant configuration for permission in override
…ble regarding RESOURCE_HITL_DETAIL, remove redundant read RESOURCE_HILT_DETAIL from test_security
75d7756 to
fffaeea
Compare
|
Thanks everyone's time on reviewing this PR. I've made the following updates to resolve the compatibility test failures.
@Lee-W , @pierrejeambrun , @vincbeck , and @jason810496 feel free to let me know if you have further feedback. I also did some manual functional tests by running Airflow with HITL example DAG. First, I defined the following roles and assign the roles to the With Test access to an GET endpoint
Test access to a PATCH endpoint
Test access to a PATCH endpointWith |
|
Looks great! Let's merge it! |





Close: #53874
HITL_DETAILin this case, and can be added to theDagAccessEntity.dependencies=[Depends(requires_access_dag(method="PUT", access_entity=DagAccessEntity.HITL_DETAIL))], to check if the user had the access toHITL_DETAILsub-entity.DagAccessEntity.HITL_DETAILis mapped to a resource typeRESOURCE_HITL_DETAIL.RESOURCE_HITL_DETAILneed to be defined inproviders/fab/src/airflow/providers/fab/www/security/permissions.pyand be configured inproviders/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py(with actions "can read", "can edit"). After the configuration, the resource type will show up inab_view_menu.^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.